Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

More efficient s3 paging #2780

Merged
merged 8 commits into from
Jun 16, 2017
Merged

Conversation

deverton
Copy link
Contributor

This PR should speed up S3 backends that have a lot of subobjects. Instead of grouping folders on the client side, it asks S3 to do the work before returning the response.

The appendIfMissing function is used by several physical backends so
move the function to a common file.
The ListObjectsV2 API allows you to specify a folder delimiter when
listing objects in a bucket. This makes S3 group the folders together
and only list the objects in the immediate prefix.

For prefixes with a large number of objects underneath, this can speed
up querying of intermediate paths significantly.
This allows you to run the S3 integration tests with the
~/.aws/credentials file and AWS_PROFILE environment variable.
@deverton
Copy link
Contributor Author

deverton commented Jun 5, 2017

I realise I forgot to explain the credentials handling changes in the test. Previously the AWS access keys had to be in the environment. Now they can be in the environment or in the ~/.aws/credentials file. This better matches the lookup path of AWS tools and means I can also use STS generated credentials.

Also, here's the acceptance test output:

$ make testacc TEST=./physical TESTARGS="-run TestS3Backend*"
==> Checking that code complies with gofmt requirements...
go generate
VAULT_ACC=1 go test -tags='vault' ./physical -v -run TestS3Backend* -timeout 45m
=== RUN   TestS3Backend
--- PASS: TestS3Backend (12.14s)
PASS
ok  	github.com/hashicorp/vault/physical	12.178s

@jefferai jefferai added this to the 0.7.3 milestone Jun 5, 2017
if os.Getenv("AWS_ACCESS_KEY_ID") == "" || os.Getenv("AWS_SECRET_ACCESS_KEY") == "" {
t.SkipNow()
}
creds := credentials.NewChainCredentials(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please look at using helper/awsutil for this as it has unified logic.

@@ -0,0 +1,10 @@
package physical

func appendIfMissing(slice []string, i string) []string {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than do this, can you make this a method in helper/strutil? There's an existing method to do the search.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. I totally missed the helper/strutil package when I did this originally. This is much better.

if os.Getenv("AWS_ACCESS_KEY_ID") == "" || os.Getenv("AWS_SECRET_ACCESS_KEY") == "" {
t.SkipNow()
}
credsConfig := &awsutil.CredentialsConfig{}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These environment variables were guarding the test from running on a system without credentials. The travis tests are failing because these tests are running now. I don't think the below error check is working

@jefferai jefferai modified the milestones: 0.7.3, 0.7.4 Jun 8, 2017
@jefferai jefferai merged commit 246602d into hashicorp:master Jun 16, 2017
@jefferai jefferai modified the milestones: 0.7.4, 0.8.0 Jul 24, 2017
@deverton deverton deleted the more-efficient-s3-paging branch March 26, 2018 01:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants